feat: add pull-based vesting pallet - #646
Conversation
Add pallet-vesting (runtime index 22): a pallet-owned pot endowed at
genesis holds the entire vesting allocation and pays beneficiaries by
plain keep-alive transfers at claim time. No locks, freezes, or holds
ever touch a beneficiary account, so wormhole addresses can be
beneficiaries and the cancel-and-repurchase double-spend of the earlier
lock-based draft is impossible by construction.
- Schedules keyed by sequential u64 ids (any number per account):
{beneficiary, start, cliff, end, total, claimed} in wall-clock ms;
linear vesting with cliff, 256-bit exact math, floor rounding with
exactness at end.
- claim(schedule_id) is permissionless; the payout always goes to the
stored beneficiary. This is the only claim path for keyless wormhole
addresses and high-security accounts (claim is HS-whitelisted).
- Admin (treasury account via EnsureTreasury, Root as break-glass):
create_schedule funds the pot from the treasury atomically,
end_schedule pays unpaid vested to the beneficiary and returns the
unvested remainder to the treasury, retarget_schedule recovers lost
keys.
- Genesis presets endow the pot with sum(totals) + ED (ED buffer even
with an empty table, as on planck) and keep the keyless pot out of
the wormhole endowment list; genesis build panics on any mismatch.
- Wormhole proof-recorder extension statically pre-charges vesting
payout transfers; claim payouts are recorded into the ZK tree like
any other transfer.
- Benchmarked weights, 44 pallet tests, runtime preset build tests,
and integration tests covering the real treasury-multisig admin flow.
spec_version 141 -> 142.
Note: the genesis pre-mine raises total issuance, reducing every future
block reward by sum(totals) / EmissionDivisor. The mainnet genesis
preset (4-of-6 treasury multisig) ships in a separate PR.
n13
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES — three blocking correctness and deployment issues remain in the wormhole vesting path.
Findings:
-
[P1] Quantize payouts before advancing
claimed(pallets/vesting/src/lib.rs:247-256). Wormhole leaves commitamount / 10_000_000_000, while permissionlessclaimtransfers every positive raw-planckowedamount and advancesclaimedby it. The included 10,000 UNIT/year schedule accrues only 3,805,175,038 planck per 12-second block: above the 1,000,000,000-planck ED, but below one wormhole quantum. A third party can therefore claim every block, move funds irreversibly into the keyless beneficiary, and create zero-value leaves that cannot recover those funds. Require totals and every beneficiary payout to be quantum-aligned, advanceclaimedonly by the transferred representable amount, define early-end dust handling, and add a repeated-third-party-claim test against the actual leaf/exit amount. -
[P1] Record payouts made through the Root admin path (
runtime/src/configs/mod.rs:571,pallets/vesting/src/lib.rs:322-336). Root governance calls are enacted by the scheduler outside any signed-extrinsic lifecycle, but vesting relies onWormholeProofRecorderExtension::post_dispatchto scan transfer events. A Rootend_schedulecan consequently pay a keyless wormhole beneficiary without inserting a leaf, leaving the vested payout unspendable. Move proof recording to a path that runs for every dispatch origin (without double-recording signed paths), and cover a scheduled Root end in an integration test. -
[P1] Initialize vesting state on runtime upgrade (
runtime/src/lib.rs:269-270,pallets/vesting/src/lib.rs:288-291,pallets/vesting/src/lib.rs:442-445). Heisenberg and Planck are upgraded in place, so adding pallet index 22 does not rerun the edited genesis presets. The new pallet arrives with a zero-balance pot and no schedules:create_schedulereturnsPotUnderfunded,try_staterejects the missing ED buffer, and Heisenberg does not receive the advertised preset schedules. Add and test an upgrade migration, or provide an explicit reset/manual initialization path that establishes the same invariant before the pallet is used.
Validation:
cargo test --locked -p pallet-vesting --features runtime-benchmarks— 44 passed.cargo test --locked -p quantus-runtime --test mod governance::vesting— 4 passed.cargo test --locked -p quantus-runtime --lib wormhole_proof_recorder_counts_vesting_calls— passed.SKIP_WASM_BUILD=1 cargo check --locked -p quantus-runtime --features runtime-benchmarks,try-runtime— passed.cargo +nightly fmt --all -- --checkandgit diff --check— passed.- All GitHub checks, including both Linux and macOS build/test matrices, are green on
a86b796afd8710a742780d5a720b714207a66c58.
The test coverage is substantial, but it does not exercise payout quantization, hook-dispatched Root payouts, or pre-existing-chain upgrade state; those gaps leave the issues above blocking.
Address review findings on the vesting pallet: - Quantize payouts to the wormhole leaf quantum (SCALE_DOWN_FACTOR, 10^10 planck): leaves commit amount/quantum, so a sub-quantum payout would be committed as a zero-value leaf and strand funds on a keyless beneficiary. Schedule totals must now be quantum-aligned, every payout is rounded down to a quantum multiple, and claimed advances only by the paid amount (stays aligned; the final claim at end remains exact). end_schedule sends sub-quantum vested dust to the treasury, which is signature-controlled and needs no leaf. Includes a regression test for the reviewed griefing scenario (per-block accrual above the ED but below one quantum). - Record payouts through the canonical TransferProofRecorder inside the pallet: transfer and proof recording are fused into a single pay_out helper, so payouts create ZK-tree leaves on every dispatch origin — including Root calls enacted by the scheduler, which run outside the signed-extrinsic lifecycle and are invisible to the event-scanning extension. The extension now skips pot-touching transfer events (no double-recording on signed paths) and no longer statically counts vesting calls; the recording cost lives in the pallet's re-benchmarked weights, with the depth-dependent ZK-tree augmentation following the reversible-transfers pattern. An integration test drives end_schedule through the real scheduler as Root and asserts the payout leaf. - No upgrade migration, by decision: this pallet ships on fresh chains whose genesis endows the pot. If it ever landed on a live chain in place, create_schedule fails loudly with PotUnderfunded until the treasury sends the pot its ED buffer — documented and covered by a bootstrap test.
|
Addressed all three findings in 5c628a7: 1. Payout quantization (fixed). New 2. Root-path proof recording (fixed). Payouts now go through the canonical 3. Upgrade migration (resolved as not applicable, per deployment model). These chains launch fresh — genesis endows the pot and seeds the schedule table, so there is no in-place-upgrade path for this pallet and a migration would be dead code. The decision is documented on the storage version, and the unfunded-pot state is not a brick: Validation: 52 pallet tests (incl. benchmark suite), 33 runtime lib tests, 33 integration tests, |
n13
left a comment
There was a problem hiding this comment.
Verdict: APPROVE (opinion) — I found no remaining blocking issues on 5c628a7d957b1765b4f964d51b35859d56f6c8ab.
The follow-up resolves all three findings from my previous review:
- Claims and early termination now quantize beneficiary payouts to the wormhole leaf quantum, advance
claimedonly by the amount actually paid, return early-end dust safely to the treasury, and cover the repeated eager-claim scenario. - Vesting records pot-to-beneficiary proofs directly, so scheduler-enacted Root dispatches create leaves; the transaction extension excludes pot flows to avoid duplicate records, and the pallet weights account for the depth-dependent tree work.
- The fresh-chain-only/no-migration policy and manual ED bootstrap for an unfunded pot are explicit, fail loudly, and have regression coverage.
Validation on the reviewed head:
cargo test --locked -p pallet-vesting --features runtime-benchmarks— 52 passed.cargo test --locked -p quantus-runtime --test mod governance::vesting— 5 passed.cargo test --locked -p quantus-runtime --lib wormhole_proof_recorder_ignores_vesting_calls_and_pot_events— passed.SKIP_WASM_BUILD=1 cargo check --locked -p quantus-runtime --features runtime-benchmarks,try-runtime— passed.cargo +nightly fmt --all -- --checkandgit diff --check— passed.
GitHub had not yet reported CI checks for this new head at review time.
Resolve genesis_config_presets conflict: keep vesting genesis schedules and pot endowment; drop wormhole endowed_addresses (proofs now derive from balances at block 1 per #648).
There was a problem hiding this comment.
Kimi Max Review — adversarial pass over the vesting pallet
Ran the full suites locally on feat/vesting-pallet (943e698): 48/48 pallet tests, 37/37 runtime lib tests (incl. all preset genesis builds + extension tests), 33/33 integration tests (incl. all 5 governance::vesting tests). Then tried to break it from every angle I could think of. One blocking finding, all in weights; the pallet logic itself held up.
Blocking
1. claim/end_schedule weights omit the depth-scaling Poseidon hash time — pallets/vesting/src/weights.rs:40 (claim_weight) and :52 (end_schedule_weight).
The augmentation adds live-depth tree DB ops (insert_leaf_db_ops()) but not the hash compute that pallet-zk-tree explicitly says must accompany every leaf-insert pricing: "Anything that prices a leaf insert must charge this in addition to Self::insert_leaf_db_ops" (pallets/zk-tree/src/lib.rs:248-252, insert_leaf_hash_ref_time()). The sibling code this PR touches — WormholeProofRecorderExtension::per_transfer_weight (runtime/src/transaction_extensions.rs:137-146) — does include it, pinned by the per_transfer_weight_includes_tree_hash_compute regression test. The vesting weights reintroduce the same omission.
Magnitude: uncharged (min(depth+1, 32) + 2) × POSEIDON_EVAL_REF_TIME_PS (50 µs padded) per payout-recording call. At tree depth 10 that's ~600 µs undeclared against a 114 µs benchmarked base for claim — a >5x under-declaration that grows with the tree, eroding the block-weight DoS bound exactly as the tree gets big. The benchmarked base only captures hash time at benchmark-time (near-empty) depth, so it doesn't cover this.
Fix is mechanical:
// in claim_weight / end_schedule_weight, next to the db ops:
.saturating_add(Weight::from_parts(pallet_zk_tree::Pallet::<T>::insert_leaf_hash_ref_time(), 0))(and the _at_depth(MAX_TREE_DEPTH) equivalent in the () impl). Note scripts/regenerate_weights.sh overwrites this file wholesale — the header comment warns about the augmentation, but it's worth double-checking after the next regen that both the DB-op and hash-time terms survive.
Non-blocking
2. Admin-call benchmarks measure the Root path, not the production path — pallets/vesting/src/benchmarking.rs:25-27. EitherOfDiverse::<EnsureRoot, EnsureTreasury>::try_successful_origin() resolves left-first to EnsureRoot → Root (frame/support/src/traits/dispatch.rs:369, pallets/frame-system/src/lib.rs:1324), so create_schedule/end_schedule/retarget_schedule are benchmarked as Root. Production dispatches via the treasury multisig as Signed(treasury), which additionally runs EnsureTreasury::try_origin → one extra TreasuryAccount storage read (runtime/src/configs/mod.rs:559-568) that no benchmark captures. ~1 read under-declared per admin call. Marginal, but the benchmarked origin should be the more expensive of the two paths.
3. PoV term counts only tree reads — weights.rs:45-46,57-58: tree_reads × TREE_KEY_POV, but insert_leaf also writes ~d+5 keys that enter the proof. Undercount ≈ (d+5) × 2600 B at depth d. Still strictly more careful than the extension (which charges no PoV here), so take it or leave it.
4. PR-body drift: the description says count_transfers "statically pre-charges claim/create_schedule (1) and end_schedule (2, worst case)" — the code deliberately charges 0 for all vesting calls since the pallet self-records (runtime/src/transaction_extensions.rs:207-211). Description-only; worth updating so the merged description matches the design.
5. Nit: genesis build early-returns on an empty schedule table (pallets/vesting/src/lib.rs:216-218), skipping the pot-endowment assertion — a hand-rolled spec with schedules = [] and an unfunded pot would build fine (fails loudly later at create_schedule with PotUnderfunded, which is documented). Shipped presets endow the ED unconditionally, so no live issue; just noting the assertion gap.
Attacked and found sound
- Exactly-once proof recording on every dispatch path: signed
claim(pallet records; extension skips pot-touching events), multisig-wrapped admin calls, scheduler-enacted Rootend_schedule(no extension runs in hook context; pallet still records — integration-tested),create_schedule(no leaf for the keyless pot — correct). Since wormhole exits mint, a double-record would be unbacked mint capacity; I found no path that produces one, and no path that loses a payout leaf. - Quantization closure: totals,
claimed, payouts, and refunds are all quantum-aligned by construction ⇒ final claim atendis exact, and every nonzeroend_schedulerefund is ≥ quantum. Sincequantum = 10^10 > ED = 10^9, theBelowMinimumfailure mode exercised in the mock is unreachable in production;end_schedulecan't be permanently bricked by a dust payout. - Pot invariant
pot ≥ Σ(total − claimed) + EDholds across all four calls;create_schedulerolls back atomically on treasury shortfall (tested);Preservation::Preservekeeps the ED buffer;beneficiary == potandtreasury == potboth blocked. - Vesting math: 256-bit rational, monotone, floor with exactness at
end; no overflow at domain extremes (tested atu64::MAXtimes). - Miner time manipulation: closed — the local timestamp fork bounds future drift to 30 s (
MAX_TIMESTAMP_DRIFT_MILLIS,pallets/timestamp/src/lib.rs:306-321). - HS whitelist:
claimis safe — payout target fixed by storage, never the caller. - PalletId
qvestingunique in the runtime; index 22 free;spec_versionbumped 141→142;std/runtime-benchmarks/try-runtimefeature wiring complete;RUNTIME_SURFACE.mdmatches the code (incl. vacant 10/12). record_transfer_proof'sboolis provably alwaystrueon these paths (amount is non-zero by the call-site guards,asset_idisNone), so ignoring it is fine here.retarget_schedulepreservesclaimed(documented); the lost-key race (old key pingsclaimbefore retarget lands) is inherent to the remedy, not a flaw.
Verdict: REQUEST_CHANGES (posted as a comment — GitHub won't let the PR author request changes on their own PR) — solely for finding 1. It's a two-line fix with an existing pattern and regression test to copy from the extension; everything else is polish. The pallet design itself is tight — the pull-based pot model closes the double-spend class by construction, and the test coverage (including the scheduler-enacted Root path) is genuinely thorough.
There was a problem hiding this comment.
CODEX Max Review
Verdict: REQUEST_CHANGES — follow-up/correction to my earlier exact-head review. Quantum alignment makes a payout representable in a leaf, but the real 4-bps circuit shows that one quantum is still not spendable. I found that blocker plus an unbounded fragmentation path, the previously noted depth-dependent weight omission, and a recovery-ordering race.
Blocking findings
-
[P1] A one-quantum vesting leaf cannot produce any positive Wormhole output (
pallets/vesting/src/lib.rs:276-305,360-390,448-457;runtime/src/configs/mod.rs:530-535,671-677).claimaccepts every non-zeroquantize_down(owed),end_scheduleuses the same quantization, and schedule validation allowstotal == PayoutQuantum. WithPayoutQuantum = SCALE_DOWN_FACTOR, such a leaf has circuitinput_amount = 1. The pinned Wormhole circuit enforces:(output_1 + output_2) * 10000 <= input * (10000 - fee_bps)At the runtime's 4 bps, the smallest positive output would require
10000 <= 9996, which is false. The only valid total output is zero. Becauseclaimis permissionless, a third party can force a payout exactly when one quantum has accrued, moving the beneficiary's funds from the pot into a leaf that cannot be exited under the current circuit. This recreates the stranding issue even though the leaf is non-zero.Require every emitted beneficiary leaf to be spendable, not merely representable. With a positive fee that means at least two quanta, and payout selection must also ensure the remaining obligation is either zero or independently spendable: e.g. paying two quanta from a three-quantum schedule leaves a final unusable quantum. Apply the same invariant to
end_schedule, reject totals that cannot satisfy it, and test the real circuit fee inequality/exit rather than only the mock proof recorder. -
[P1] Permissionless claims allow an attacker to fragment a grant into an unbounded number of proof obligations (
pallets/vesting/src/lib.rs:276-305,448-457;runtime/src/genesis_config_presets.rs:70-78;pallets/wormhole/build.rs:39-52).There is no minimum economic tranche, cadence, or maximum claim count. A valid no-cliff grant matching the example
10_000 * UNITtotal contains 1,000,000 payout quanta, and the one-year duration has enough blocks to emit them separately. At the compiled defaults (NUM_LEAF_PROOFS = 7,NUM_PRIVATE_BATCH_PROOFS = 53), fully processing that fragmentation means roughly 1,000,000 leaf proofs, 142,858 private batches, and 2,696 public batches. The caller pays transaction fees but imposes the proof workload and note management on the beneficiary; a block producer can also self-include the griefing claims.Raising the spendability floor to two quanta alone still permits 500,000 leaves. Add a distinct bound such as
MaxClaims, a minimum economic tranche/claim cadence, or a total-relative tranche size, and assert the worst-case leaf count in tests. -
[P1]
claimandend_scheduleomit the required depth-scaling Poseidon compute charge (pallets/vesting/src/weights.rs:36-60,69-72,88-90,102-120;pallets/zk-tree/src/lib.rs:84-100,242-255).Vesting adds
insert_leaf_db_ops()but notinsert_leaf_hash_ref_time(), despitepallet-zk-treeexplicitly requiring both for every leaf insertion. At maximum depth the omitted term is(32 + 2) * 50_000_000 = 1_700_000_000ps per payout-recording call. The transaction extension deliberately excludes these pot flows, so no other layer charges it. Permissionless underweighted claims can erode the block execution bound as the tree deepens.Add the live-depth hash
ref_timeto both runtime weights and the max-depth equivalent to the()implementation, then pin depth growth with a zero-DB-weight regression test. The admin weights also need the worst valid origin:AdminOrigin::try_successful_origin()selects Root first (benchmarking.rs:25-27), while the production signed-treasury path readsTreasuryAccountinEnsureTreasury(runtime/src/configs/mod.rs:553-568). That extra read is absent from the generated admin-call weights, includingretarget_schedule's one-read total. -
[P2] A public retarget can be front-run by permissionless
claim, defeating lost-key recovery for already-vested unpaid value (pallets/vesting/src/lib.rs:276-305,393-415).Retarget is documented as the remedy for a lost key, but it changes only the stored beneficiary and preserves
claimed. Once a treasury multisig retarget is visible in the proposal/mempool, anyone can callclaimfirst. That pays all currently vested value to the old lost or compromised account and advancesclaimed; the later retarget only protects the remainder. The pinger does not receive the funds, but can irreversibly choose the race outcome for the beneficiary.Define and enforce deterministic recovery semantics. If all unpaid value is recoverable, retarget needs ordering/pause protection. If vested value belongs irrevocably to the old account, retarget should settle that entitlement itself so an unrelated caller cannot decide the outcome, and the limitation should be explicit and tested.
Validation
cargo test --locked -p pallet-vesting --features runtime-benchmarks— 52 passed.cargo test --locked -p quantus-runtime --test mod governance::vesting— 5 passed.cargo test --locked -p quantus-runtime --lib wormhole_proof_recorder_ignores_vesting_calls_and_pot_events— passed.cargo test --locked -p quantus-runtime --lib genesis_config_presets— 4 passed.SKIP_WASM_BUILD=1 cargo check --locked -p quantus-runtime --features runtime-benchmarks,try-runtime— passed.cargo +nightly fmt --all -- --checkandgit diff --check— passed.- All GitHub checks are green on
943e69835a526149fa9013fa1b7cfe3064970b09.
I rechecked dispatch atomicity, proof-recording paths, origins, vesting arithmetic, pot accounting, genesis presets, and the accepted fresh-chain/no-migration policy. Those areas held up; the findings above are cross-pallet/circuit invariants not exercised by the green tests.
There was a problem hiding this comment.
Round 2 review — xhigh adversarial pass at df8a0cc0
Method: 10 independent finder angles over the full diff + enclosing code, 1-vote verification per candidate (mechanicals re-checked by hand against vendored FRAME sources), then a gap sweep. Round-1 findings re-checked: the Poseidon hash-time augmentation is in (weights.rs:28-29), benchmarks now dispatch signed-treasury (benchmarking.rs:28-33), PoV covers tree writes — all fixed. Local test run at this head: 56/56 pallet tests pass. Findings below, ranked.
Blocking / should-fix
1. end_schedule refunds to the treasury without the treasury != pot guard that create_schedule has — pallets/vesting/src/lib.rs:436
pallet_treasury accepts any non-zero account (pallets/treasury/src/lib.rs:163, genesis too). If the treasury is ever set to the vesting pot and an admin ends a mid-vesting schedule: pay_out sends the vested part to the beneficiary and records the leaf, then transfer(&pot, &treasury=pot, remainder, Preserve) hits the source == dest early-return Ok no-op (frame/support/src/traits/tokens/fungible/regular.rs:330-334) — no funds move, no Transfer event — the schedule is then deleted, ScheduleEnded.unvested_returned misreports the remainder as treasury-bound, and the funds sit stranded on the keyless pot (recoverable only by Root). do_try_state passes (pot over-covered), so nothing trips. Fix: apply the same guard in end_schedule.
2. do_try_state requires pot ≥ ED even with zero schedules, contradicting the documented "unfunded pot is supported" state — pallets/vesting/src/lib.rs:640
planck/heisenberg are live chains running this runtime; spec 141→142 adds the pallet in place, genesis never re-runs, so Schedules is empty and the pot balance is 0. lib.rs:96-103 explicitly blesses that state ("fails loudly … until the treasury sends the pot its existential-deposit buffer") and genesis early-returns on empty tables (lib.rs:244), but do_try_state computes required = 0 + ED and errors "pot does not cover outstanding obligations" on every try-runtime block check against the upgraded chain until someone endows the pot. Fix: require the ED buffer only when schedules exist (or drop the "supported unfunded pot" claim).
3. Committed weights were generated against a debug-built runtime wasm — pallets/vesting/src/weights_generated.rs:31
The header's executed command (release quantus-node + --runtime=target/debug/wbuild/quantus-runtime/quantus_runtime.wasm, CPU <UNKNOWN>) contradicts scripts/regenerate_weights.sh, which targets the release wasm (line 6). All four base ref_times were measured on unoptimized wasm and are systematically inflated — vesting extrinsics are over-charged (safe direction: no block-time risk, but fees are overpriced and block capacity underused), and the committed artifact contradicts the repo's own regeneration procedure. Regenerate per the script.
4. The claim benchmark measures the cheapest path — pallets/vesting/src/benchmarking.rs:71
It uses set_time(END) with last_claim_at: None, so the now >= end early-return skips vested_amount's 256-bit rational mul/div, the max_non_final reserve arithmetic, and the rate-limit check. The common real-world claim is mid-vesting on a schedule with a prior claim — strictly more ref_time than the benchmarked worst case, so WeightInfo::claim() under-declares the common path. Fix: benchmark at set_time(END/2) with a prior claim, mirroring end_schedule/retarget_schedule.
7. The canonical runtime-surface doc (a PR file) drifted from the head commit on five points — docs/RUNTIME_SURFACE.md:182
(a) Schedules layout omits last_claim_at — an indexer hand-decoding per this doc misdecodes every schedule; (b) line 187 says genesis validates total ≥ ED, but code requires total ≥ MinimumPayout (= UNIT, 1000× ED) — an operator preparing the planned mainnet table per the doc gets a genesis panic; (c) line 186 says retarget "changes the beneficiary key only" — it now settles the full claimable payout to the OLD beneficiary and emits vested_paid; (d) the claim bullet omits MinimumPayout, the final-claim reserve, and the 24h MinClaimInterval — a claim UI built on it promises payouts the chain rejects with ClaimTooSoon/ClaimWouldLeaveDust; (e) line 250 says the pot is kept "out of the wormhole endowment list" while genesis_config_presets.rs's own comment says the pot gets a block-1 leaf (unspendable).
8. assert_eq!(x, true) trips clippy::bool_assert_comparison — pallets/vesting/src/tests.rs:113
Reproduced locally: cargo clippy -p pallet-vesting --all-targets --all-features emits the lint, and pm-quality-check runs clippy --all-targets --all-features with -D warnings per crate (.github/workflows/pm-quality-check.yml:113). If the per-crate gate runs for this new crate, the PR goes red. One-line fix: assert!(...).
Non-blocking
5. count_transfers statically charges 1 for plain Balances transfers INTO the pot that the scan deliberately skips — runtime/src/transaction_extensions.rs:183 vs :253
Treasury sending the pot its ED buffer (the documented manual bootstrap on an unfunded-pot chain) is charged TransferCount r/w + depth-scaled insert_leaf_db_ops + insert_leaf_hash_ref_time, the scan drops the event (to == pot), and the reconciliation never refunds — the sender pays for a leaf insert that never runs, and the overcharge grows with tree depth. Fix: check the dest against the pot in count_transfers (rare op, so acceptable to wontfix, but note it).
6. PayoutBelowMinimum locks end_schedule for essentially the whole vesting period of a total == MinimumPayout schedule — pallets/vesting/src/lib.rs:422
Treasury creates a 1-year schedule with total = 1 UNIT (= MinimumPayout, smallest valid). Once ≥1 quantum vests, 0 < vested_paid < MinimumPayout holds until end, so end_schedule fails for the entire middle of the schedule — the treasury's funds are locked with no admin recourse but waiting. A payout in [quantum, minimum) is leaf-safe (commits a non-zero leaf); the safety floor is PayoutQuantum, while MinimumPayout is a claim-path anti-spam rule. Fix: guard with >= PayoutQuantum (or exempt the case vested_paid == remaining), keeping the minimum on claim only.
9. Mock's mutable pub static config and RECORDED_PROOFS are never reset — latent cross-test leak — pallets/vesting/src/mock.rs:40
ExistentialDeposit, TreasuryAccount, PayoutQuantum, MinimumPayout, MinClaimInterval statics and the thread_local RECORDED_PROOFS are mutated by ~10 tests. Rust's harness reuses worker threads across tests; a TreasuryAccount::set(None) or PayoutQuantum::set(3_000) persists into the next test on the same worker unless that test sets it first, and proof-recording tests asserting exact recorded() vectors accumulate earlier entries — order-dependent flakes that pass today by scheduling luck. Fix: reset statics to defaults and clear RECORDED_PROOFS in new_test_ext.
10. No ceiling on payout size vs the ZK leaf's u32 quantized-amount clamp — pallets/vesting/src/lib.rs:586
hash_leaf saturates amount / 10^10 at u32::MAX (pallets/zk-tree/src/tree.rs:148). A schedule with total > ~42.95M UNIT (schedule_is_valid has no upper cap) vests fully; the final claim pays it in one transfer and records one leaf clamped at u32::MAX quanta; the excess over the clamp is backed by the real transfer but has no exitable leaf — stranded on a keyless beneficiary (conservative direction, no unbacked mint). Pre-existing tree limitation, but vesting's one-shot final payouts reach it sooner than everyday transfers. Cap total per schedule or document the ceiling.
11. Manual weights augmentation is brittle — pallets/vesting/src/weights.rs:17
BENCHMARK_TREE_READS=5/WRITES=4 are untethered from the generated file, saturating_sub clamps any mismatch silently, and the benchmark-depth PoV/hash embedded in the base are double-counted (overcharge direction). A zk-tree refactor that changes tree-op counts in a regenerated weights_generated.rs leaves payout_weight over/under-correcting with no compile error and no failing test (the only test checks monotonicity). Pin the constants to the generated file's storage table in a test, or benchmark with a pre-grown tree so no subtraction is needed.
12. Each payout weight evaluation reads ZkTree::Depth twice — pallets/vesting/src/weights.rs:45
insert_leaf_db_ops() and insert_leaf_hash_ref_time() each call Depth::<T>::get() internally. Every get_dispatch_info for claim/end/retarget performs two identical trie reads where one suffices: read d = Depth::<T>::get() once and call the _at_depth(d) variants (the () impl's own pattern).
13. Per-dispatch waste — runtime/src/transaction_extensions.rs:240
The extension derives the pot account (Blake2b hash) on every successful extrinsic even with zero Transfer events — the overwhelming majority of extrinsics — solely so the filter can skip pot events that never occur; derive lazily inside the Transfer arm. configs/mod.rs:561 reads treasury_account() in EnsureTreasury::try_origin, then lib.rs:364/:415 re-read the same key (EitherOfDiverse::Success already carries the AccountId on the signed arm). lib.rs:458 and :465 each hash the pot id.
14. DRY cluster (user-level coding rule "Duplicate code must be avoided at all costs")
(a) Depth-aware leaf-insert pricing is re-composed in reversible-transfers weights, wormhole weights, the extension's per_transfer_weight, and now payout_weight — belongs as one helper in pallet-zk-tree (a cost-model change updated in 3 of 4 places silently misprices the 4th). (b) VestingPayoutQuantum anchors to pallet_wormhole::SCALE_DOWN_FACTOR while the actual leaf quantum is pallet_zk_tree::tree::AMOUNT_SCALE_DOWN_FACTOR — if they ever diverge, sub-quantum payouts strand funds; add at least a const assert tying them. (c) claim's payout block (lib.rs:338-346) and retarget's settle block (lib.rs:463-475) duplicate pay_out + claimed + last_claim_at. (d) MockProofRecorder is the 3rd copy (mining-rewards, reversible-transfers) instead of a shared std-gated helper in qp-wormhole; governance/vesting.rs:23's account() duplicates TestCommons::account_id (runtime/tests/common.rs:9); 86_400_000 appears 3× in the runtime crate.
15. Design note: the pot-skip hardcodes a per-pallet exemption in runtime-wide transaction infrastructure — runtime/src/transaction_extensions.rs:253
A future pallet-grants with its own keyless pot copying the vesting pattern must also edit this skip; forgetting yields double-recorded payouts (two exitable leaves per real transfer = unbacked mint capacity). Deeper fixes: a recorder-side registry the extension queries, or eventless increase_balance payouts (the mining-rewards pattern) that need no extension special case.
Refuted along the way (checked, not bugs)
- Retarget skipping settlement on TooSoon/WouldLeaveDust — matches the documented contract ("settle any payout a permissionless claim could force";
claim_planis shared, so settle ⇔ claimable by construction). - The
use qp_wormhole::TransferProofRecorderimport — used by theT::ProofRecorder::record_transfer_proofcall syntax; cargo check clean. - Missing pot guards on
TransferOnHold/ReserveRepatriated/Mintedarms — no path can hold/reserve/mint on the keyless pot. pay_outignoring the recorder's bool —falseis unreachable (amount ≥MinimumPayout> 0, native asset); anensure!would be defense-in-depth only.end_schedulearithmetic and refund alignment — refund is always a quantum multiple > ED,BelowMinimumunreachable.- Timestamp monotonicity; pot-solvency invariant across all four calls; exactly-once recording on signed/batch/multisig/scheduler-Root paths; mock/runtime/benchmark constant divergence — intentional, benchmarks derive from
T.
Verdict: REQUEST CHANGES (posted as a comment — GitHub won't let the author request changes on their own PR). Round 1's blocker is properly fixed; this round's should-fix set is findings 1–4, plus 7 (doc drift in a PR file) and 8 (clippy gate) as cheap hygiene. Findings 5, 6, 9–15 are non-blocking polish. The pallet core — vesting math, quantization closure, pot invariant, exactly-once recording, claim_plan — held up under everything I threw at it.
|
Fixed the four Round 2 blocking findings in
Validation completed:
|
n13
left a comment
There was a problem hiding this comment.
Round 3 — fix verification at c1c1b9fe
Re-checked the four round-2 blockers against the fix commits (e142ed23, c1c1b9fe), all locally verified:
treasury != potguard — FIXED. Sharedtreasury_and_pot()now gates bothcreate_scheduleandend_schedule(lib.rs:490-495), and the regression test proves a pot-aliased treasury failsend_schedulewithout touching schedule, beneficiary, or pot state.- try-state vs unfunded pot — FIXED. The ED buffer is only required when schedules exist (
lib.rs:641-648);empty_is_a_noopnow provesdo_try_statepasses with a zero-balance pot, matching the documented "unfunded pot degrades loudly" state live chains get from the in-place upgrade. - Debug-wasm weights — FIXED.
weights_generated.rswas regenerated against the release wasm (header now showstarget/release/wbuild/...), per the repo script. - Claim benchmark measuring the cheapest path — FIXED. The benchmark now performs a setup claim at
MinClaimIntervaland measures a second claim at2×intervalon a schedule ending at4×interval(benchmarking.rs:71-93): the 256-bit mul/div, the reserve branch, and the rate-limit branch all execute in the measured path, with assertions pinningclaimed_before < claimed < total. The regenerated storage lists match the newCLAIM_BENCHMARK_TREE_WRITES = 3/BENCHMARK_TREE_WRITES = 4split (claim's insert doesn't grow Depth; end/retarget's does).
Local runs at this head: 57/57 pallet tests, 37/37 runtime lib tests (all preset genesis builds), 34/34 integration tests (all 6 governance::vesting tests, incl. the new 1-QUAN boundary and payout-policy tests) — all pass.
Still open, all non-blocking (from round 2; none re-introduced or worsened):
- #7 doc drift —
docs/RUNTIME_SURFACE.mdis a PR file and still predates the head-commit behavior: storage layout missinglast_claim_at,total ≥ ED→ code requirestotal ≥ MinimumPayout, retarget "changes key only" → settles first, claim rules (minimum/reserve/24h) undocumented, "pot out of wormhole endowment list" vs pot getting a block-1 leaf. Worth a pass before merge — indexers and mainnet-preset authors read this doc. - #8 clippy —
bool_assert_comparisonatpallets/vesting/src/tests.rs:113still trips the per-crate-D warningsgate if it runs for this crate; one-lineassert!(...)fix. - #5
count_transferspot-inbound overcharge, #6PayoutBelowMinimumwindow ontotal == MinimumPayoutschedules (now test-pinned on the claim side), #9 mock statics reset, #10 u32 leaf-amount clamp ceiling for >42.95M-UNIT single payouts, #11–15 weights-brittleness/efficiency/DRY/altitude notes — all polish, fine to defer.
Verdict: APPROVE (posted as a comment — GitHub won't let the author approve their own PR). All blocking findings from both rounds are fixed with regression coverage; the remaining items are documentation and hygiene. The pallet is in good shape: pull-based pot model, exact-once recording on every dispatch path, closed quantization, and a claim path that survives the lost-key, keyless-beneficiary, and scheduler-Root cases it was designed for.
Summary
Adds
pallet-vestingat runtime index 22 (spec_version142), implementing a pull-based vesting wallet. The pallet-owned pot (PalletId(*b"qvesting")) holds the unclaimed allocation and pays beneficiaries through plain keep-alive transfers only when a payout is due.No locks, freezes, or holds touch beneficiary accounts. Wormhole/keyless addresses can therefore be beneficiaries, and funds only move from the pot to the beneficiary once.
Design
u64ids and contain{beneficiary, start, cliff, end, total, claimed, last_claim_at}. An account may hold any number of schedules.cliff, linear fromstarttoend, and exactlytotalatend. The calculation uses 256-bit rational arithmetic and floor rounding.claim(schedule_id)is permissionless, but always pays the stored beneficiary rather than the caller. This supports keyless wormhole addresses and high-security accounts.Payout safety
ClaimWouldLeaveDustuntil the full remainder is vested.Administration
Admin operations use
EnsureTreasury(the configured treasury account, with Root as break-glass):create_schedulevalidates the schedule, transferstotalfrom treasury to the pot atomically, and requires the pot's existential-deposit buffer to exist.end_schedulepays the quantized unpaid vested amount to the beneficiary and returns everything else to treasury. A non-zero beneficiary payout below the minimum is rejected without removing the schedule.retarget_schedulefirst settles exactly the payout a permissionless claim could currently force to the old beneficiary, then changes the beneficiary. This makes the result independent of claim/retarget transaction ordering.Integration
devandheisenberginclude example schedules, including multiple schedules for one account and a keyless test address.count_transferspre-chargesclaimandcreate_schedulefor one transfer andend_schedulefor two transfers in its worst case.Verification
cargo test --locked -p pallet-vesting --features runtime-benchmarks— 60 passed.cargo test --locked -p quantus-runtime --test mod governance::vesting— 6 passed.cargo test --locked -p quantus-runtime --lib— 37 passed.cargo test --locked -p quantus-runtime --test mod— 34 passed, 1 pre-existing test ignored.cargo check --locked -p quantus-runtime --features runtime-benchmarks,try-runtime— passed.cargo clippy --locked -p pallet-vesting --features runtime-benchmarks --no-deps -- -D warnings— passed.cargo +nightly fmt --all -- --checkandgit diff --check— passed.claim,create_schedule,end_schedule, andretarget_schedule— passed.Notes
EmissionDivisor.